Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rollups] Add useNormalizedEsInterval param to date histogram bucket to allow rollups to avoid normalizing ES interval units #24428

Merged

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Oct 23, 2018

This PR doesn't do what it's supposed to, but I need some help figuring out why @jen-huang and @timroes.

This PR fixes an error in which narrowing the time range of a rollup vis results in an API error due to a fixed time interval being converted to a calendar time interval. To repro the error, create a date histogram visualization using rollup data, with an interval of 60m. Ensure the time range is large enough so that the visualization will render. Brush or otherwise change the time range so that it's very narrow and you get a toast error stating something like Rollup search error: [illegal_argument_exception] There is not a rollup job that has a [date_histogram] agg on field [utc_time] which also satisfies all requirements of query.

This PR is supposed to fix this error, but it doesn't because it looks like we're calling write on the date histogram bucket twice in a single render. The first time, useNormalizedEsInterval is correctly set to false but the second time it looks like it changes to true but I'm having a hard time figuring out why.

EDIT: @jen-huang and I weren't able to reproduce the bug I originally found with this solution. She tested more on her own and still wasn't able to repro. So it appears this solution works.

TODO:

  • Add rollups functional tests verifying that various time ranges do not result in this error

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor Author

@timroes Jen and I found another wrinkle we need your feedback on. It looks like date_histogram has a getFormat function which indirectly calls time_buckets getInterval function as part of the logic for initializing the X axis (see links for references to the code).

Our question is whether this should also hook into the useNormalizedEsInterval flag we're adding here, or if it's OK to leave as-is. We're thinking that the answer is yes, we will need to hook into it to ensure the values we get back from ES match what we're showing on the X axis.

…ollups to avoid normalizing ES interval units.
@cjcenizal cjcenizal force-pushed the rollups-normalize-interval-bug branch from cc4832d to 66e73da Compare October 24, 2018 16:15
@cjcenizal cjcenizal requested a review from spalger October 24, 2018 16:15
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and fix LGTM!

@spalger
Copy link
Contributor

spalger commented Oct 24, 2018

This doesn't work for me, when I look at about 2 weeks worth of data:

image

image

It also doesn't seem to work even if I'm using a smaller date range, but without the same errors:
image

@timroes
Copy link
Contributor

timroes commented Oct 24, 2018

I looked quickly at it, and I don't have any objections or seeing any obvious issues, also after talking in depth with CJ about that yesterday. Currently don't have time for a full review, so please feel free to merge this also without my complete review.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jen-huang
Copy link
Contributor

jen-huang commented Oct 24, 2018

@spalger Those almost look like time filter errors? Are you able to create a normal (non-rollup index pattern) visualization?

I have 60/1 500k makelogs data and am using the rollup job config you sent me yesterday:

image

image

Current state of my job as a sanity check. Is there some discrepancy between our setup?
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants